test(discord): Add null-guard branch coverage for BotService constructor#377
test(discord): Add null-guard branch coverage for BotService constructor#377github-actions[bot] wants to merge 2 commits into
Conversation
Adds a reflection-based test to cover the null-bot argument guard in BotService, bringing BotService to 100% line and branch coverage. The null! operator cannot be used directly due to the NX0002 analyser rule, so the constructor is invoked via reflection. Prompt: Work on issue #356 in funfair-tech/BuildBot. Closes #356
| TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => | ||
| ctor.Invoke([null, messageChannel, releaseChannel]) | ||
| ); | ||
| Assert.IsType<ArgumentNullException>(ex.InnerException); |
There was a problem hiding this comment.
[Code Review] The test asserts only IsType<ArgumentNullException> without checking ParamName. If the guard in BotService is accidentally changed to throw new ArgumentNullException(nameof(botMessageChannel)) (wrong parameter name), the test still passes and the regression goes undetected.
Suggestion: also assert the parameter name:
ArgumentNullException ane = Assert.IsType<ArgumentNullException>(ex.InnerException);
Assert.Equal("bot", ane.ParamName);| MessageChannel<BotMessage> messageChannel = new(); | ||
| MessageChannel<BotReleaseMessage> releaseChannel = new(); | ||
|
|
||
| ConstructorInfo ctor = Assert.Single(typeof(BotService).GetConstructors()); |
There was a problem hiding this comment.
[Code Review] Assert.Single(typeof(BotService).GetConstructors()) is a guard against a second constructor being added, but if one is ever added the failure message will be a count-related xunit error with no mention of null guards — making the root cause non-obvious to the next developer.
Consider selecting the specific constructor by its parameter types instead, which both locates the right overload and provides a clearer failure if the signature changes:
ConstructorInfo? ctor = typeof(BotService).GetConstructor([
typeof(IDiscordBot),
typeof(IMessageChannel<BotMessage>),
typeof(IMessageChannel<BotReleaseMessage>)
]);
Assert.NotNull(ctor);| [Fact] | ||
| public void Constructor_ThrowsArgumentNullException_WhenBotIsNull() | ||
| { | ||
| MessageChannel<BotMessage> messageChannel = new(); |
There was a problem hiding this comment.
[Code Review] messageChannel and releaseChannel are created but never reached: the BotService constructor throws ArgumentNullException on the first line (bot ?? throw ...) before it touches either channel parameter. Passing null for those positions (or any placeholder) would produce the same result and make it clearer that only bot is the subject under test.
|
[Code Review — Pre-existing gap] |
- Use GetConstructor with explicit parameter types instead of Assert.Single on GetConstructors(), avoiding confusing failure messages if a second constructor is ever added - Remove unnecessary MessageChannel allocations — the constructor throws before touching the channel parameters, so null suffices for those args - Assert ParamName == "bot" on the caught ArgumentNullException so a mis-targeted guard is detected rather than silently passing Prompt: Work on pull request #377 in funfair-tech/BuildBot.
Summary
bot == nullguard inBotServiceconstructor, bringingBotServiceto 100% line and branch coverage.null!directly, so the constructor is invoked viaConstructorInfo.Invokewith a null argument instead.Coverage status after this PR
Remaining gaps in
BuildBot.Discord(DiscordChannelAdapterand the live-infrastructure methods ofDiscordSocketClientAdapter) are infrastructure-dependent and tracked in issue #374.Closes #356
Test plan
BotServicereaches 100% line and branch coveragedotnet buildcheckpasses with no errors